Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add per user metrics to mysql input #6132

Merged
merged 10 commits into from
Dec 15, 2020
Merged

Add per user metrics to mysql input #6132

merged 10 commits into from
Dec 15, 2020

Conversation

AlexKapustin
Copy link
Contributor

@AlexKapustin AlexKapustin commented Jul 17, 2019 •
edited by danielnelson
Loading

closes #6131

Required for all PRs:

  • [ X] Signed CLA.
  • [ X] Associated README.md updated.
  • [ N/A] Has appropriate unit tests.

@danielnelson danielnelson added area/mysql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin labels Jul 17, 2019
@danielnelson danielnelson changed the title #6131 Add per user metrics to mysql input Jul 17, 2019
@AlexKapustin
Copy link
Contributor Author

Pull request was opened one year ago, and no any single comment :(
It highly "encourage and motivate" people to contribute

srebhan
srebhan reviewed Dec 3, 2020
View reviewed changes
plugins/inputs/mysql/mysql.go Outdated
// gatherPerfSummaryPerAccountPerEvent can be used to fetch enabled metrics from
// performance_schema.events_statements_summary_by_account_by_event_name
func (m *Mysql) gatherPerfSummaryPerAccountPerEvent(db *sql.DB, serv string, acc telegraf.Accumulator) error {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove newline to match style of other gather functions.

srebhan
srebhan requested changes Dec 3, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AlexKapustin! I totally agree that this is killing motivation and I want to apologize for this taking so long. InfluxData is trying to improve and here I am.. ;-P

I have some comments in the code. The most striking one is that I think the code breaks when limiting the number of collected events using the perf_summary_events option. Could you please take a look!? I promise it will not take another year to get this in, given we can work out the issues. :-/

@srebhan srebhan self-assigned this Dec 3, 2020
@sjwang90 sjwang90 added this to the Planned milestone Dec 9, 2020
@AlexKapustin
Copy link
Contributor Author

Hello, @srebhan !

Thank you very much for your response :)
No need to be sorry, thank you for reply at all :)
And sorry for my late reply. I will have a look at your comments shortly

srebhan
srebhan reviewed Dec 11, 2020
View reviewed changes
plugins/inputs/mysql/mysql.go Outdated
@@ -121,6 +123,13 @@ const sampleConfig = `
# perf_events_statements_limit = 250
# perf_events_statements_time_limit = 86400

## gather metrics from PERFORMANCE_SCHEMA.EVENTS_STATEMENTS_SUMMARY_BY_ACCOUNT_BY_EVENT_NAME
gather_perf_sum_per_acc_per_event = false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for missing this: Please comment the lines containing default values!

srebhan
srebhan reviewed Dec 11, 2020
View reviewed changes
plugins/inputs/mysql/mysql.go Outdated

## list of events to be gathered for gather_perf_sum_per_acc_per_event
## in case of empty list all events will be gathered
perf_summary_events = []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, comment that line as it contains the default.

@AlexKapustin
Copy link
Contributor Author

It's done! Thank you for your review.

srebhan
srebhan approved these changes Dec 11, 2020
View reviewed changes
Copy link
Member

@srebhan srebhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@srebhan srebhan added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label Dec 11, 2020
reimda
reimda reviewed Dec 11, 2020
View reviewed changes
Copy link
Contributor

@reimda reimda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change could also use some tests. The mysql plugin in general doesn't have much test coverage. We could mock the result of a query and make sure the output metric is as expected. Even an integration test that requires mysql to be running would be useful.

plugins/inputs/mysql/README.md Outdated Show resolved Hide resolved
@AlexKapustin @reimda
Update plugins/inputs/mysql/README.md
f0f12cb 
keep sample config consistent

Co-authored-by: reimda <[email protected]>
@srebhan
Copy link
Member

srebhan commented Dec 13, 2020

@AlexKapustin could you look into the unit tests @reimda suggested!?

@AlexKapustin
Copy link
Contributor Author

@AlexKapustin could you look into the unit tests @reimda suggested!?

Yes, I will have a look today. Thank you for remind me :)

@AlexKapustin
Copy link
Contributor Author

I had have a look, and basically saying, mysql plugin doesn't have tests at all, only connections and one integration test!
Do you, guys, think it would be good idea to separate that from this scope, create new ticket to cover all measurements by tests?
It's quite big task to do so, hence, I would prefer to create separate ticket for that.
Thoughts?

@reimda reimda mentioned this pull request Dec 14, 2020
Closed
@reimda
Copy link
Contributor

reimda commented Dec 14, 2020

Hi @AlexKapustin, thanks for being so responsive even though it took us so long to get to this PR.

It would be great if you could write tests in this PR that cover the new functionality in this PR. If this will require setting up a lot of common infrastructure and you would like to do it in a separate PR that's ok too. Whatever you prefer.

I went ahead and made a new issue about the mysql missing tests (#8561). If you would like to write the missing tests for the existing functionality it would be very appreciated!

@srebhan
Copy link
Member

srebhan commented Dec 15, 2020

Thanks @AlexKapustin for clarification. I think we can merge this one as the code looks fine... As @reimda said, tests are appreciated and I promise it won't take another year to review them. ;-)

@AlexKapustin
Copy link
Contributor Author

Hello, @reimda and @srebhan !
Thank you for your responses.
I would prefer to do tests in new ticket. I will try to come up with some tests in #8561
Thanks again!

@reimda reimda merged commit 21253ec into influxdata:master Dec 15, 2020
ssoroka pushed a commit that referenced this pull request Dec 16, 2020
@AlexKapustin @ssoroka
Add per user metrics to mysql input (#6132)
2512bb3 
(cherry picked from commit 21253ec)
arstercz pushed a commit to arstercz/telegraf that referenced this pull request Mar 5, 2023
@AlexKapustin
Add per user metrics to mysql input (influxdata#6132)
dab73f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reimda reimda reimda approved these changes

@srebhan srebhan srebhan approved these changes

Assignees

@srebhan srebhan

Labels
area/mysql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add metrics from performance_schema.events_statements_summary_by_account_by_event_name
5 participants
@AlexKapustin @srebhan @reimda @danielnelson @sjwang90